Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-102249: Expand sys.call_tracing documentation #102806

Merged
merged 10 commits into from
Oct 31, 2023
Merged

Conversation

impact27
Copy link
Contributor

@impact27 impact27 commented Mar 18, 2023

@bedevere-bot bedevere-bot added docs Documentation in the Doc dir skip news awaiting review labels Mar 18, 2023
@impact27 impact27 changed the title PR: Expend sys.call_tracing documentation gh-102249: Expend sys.call_tracing documentation Mar 18, 2023
@hugovk hugovk changed the title gh-102249: Expend sys.call_tracing documentation gh-102249: Expand sys.call_tracing documentation Mar 18, 2023
@CAM-Gerlach CAM-Gerlach added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Mar 18, 2023
@CAM-Gerlach
Copy link
Member

(Removed the Fixes tag since we only close the associated issue when all backport PRs are merged, as is SOP for docs changes like this).

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Standard reminder: You can directly apply all the suggestions you want in one go with Files changed -> Add to batch -> Commit

Don''t we also want to add a note in the setprofile and settrace descriptions referring readers to call_tracing to nest tracing/profiling an existing tracing/profiling session, to properly address the original issue that prompted the issue? Otherwise, readers may try to use these together anyway and not find call_tracing on their own right way, no?

I suggest adding a .. note:: with similar or somewhat more cut down version of the text here to each at the end of the main top-level description (before getting in to the event details).

Doc/library/sys.rst Outdated Show resolved Hide resolved
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks; I had a couple small suggested tweaks to your notes, and one recommended manual change I couldn't make as a suggestion.

Doc/library/sys.rst Outdated Show resolved Hide resolved
Doc/library/sys.rst Outdated Show resolved Hide resolved
@@ -1415,6 +1419,11 @@ always available.
``'c_exception'``
A C function has raised an exception. *arg* is the C function object.

.. note::
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't reach it with a comment, but after applying the suggested changes, I recommend moving this note above the list of events to be more consistent with the other added note and avoid getting lost there. Specifically, I suggest swapping it with the audit-event directive, which would be consistent with how both are in settrace and also avoids obvious confusion (which confused me, certainly) between the event argument and the audit event, given the latter is currently the one mentioned immediately before "the events have the following meaning" which confusing implies it is being referred to there, rather than the events discussed in the paragraph above the audit-event directive.

Comment on lines 1399 to 1404
.. note::
The same tracing mechanism is used for :func:`!setprofile` as :func:`settrace`.
To trace calls with :func:`!setprofile` inside a tracing function (e.g. in a
debugger breakpoint) see :func:`call_tracing`.

.. audit-event:: sys.setprofile "" sys.setprofile
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, sorry if I was unclear before—the note should go one block above, and the audit-event should go where the `note used to be (i.e. below the list of event values. This will avoid interjecting these two between the introduction of the profile function args and the description of their values, which is particularly confusing to readers because the audit "events" are entirely unrelated to the event argument here.

Also, the indents got off as a result of this change.

As I should have before, here's a patch you can apply with

git apply 0001-Move-call_tracing-note-audit-event-in-setprofile-to-.patch

to fix all this directly:

0001-Move-call_tracing-note-audit-event-in-setprofile-to-.patch

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Mar 23, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from me, thanks @impact27 !

@hugovk hugovk removed the needs backport to 3.10 only security fixes label Apr 7, 2023
@hugovk hugovk added the needs backport to 3.12 bug and security fixes label Oct 31, 2023
@hugovk hugovk enabled auto-merge (squash) October 31, 2023 16:13
@hugovk hugovk merged commit 2445673 into python:main Oct 31, 2023
23 checks passed
@miss-islington-app
Copy link

Thanks @impact27 for the PR, and @hugovk for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 31, 2023
(cherry picked from commit 2445673)

Co-authored-by: Quentin Peter <[email protected]>
Co-authored-by: C.A.M. Gerlach <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 31, 2023
(cherry picked from commit 2445673)

Co-authored-by: Quentin Peter <[email protected]>
Co-authored-by: C.A.M. Gerlach <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Oct 31, 2023

GH-111557 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Oct 31, 2023
@bedevere-app
Copy link

bedevere-app bot commented Oct 31, 2023

GH-111558 is a backport of this pull request to the 3.11 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.11 only security fixes label Oct 31, 2023
hugovk pushed a commit that referenced this pull request Oct 31, 2023
hugovk pushed a commit that referenced this pull request Oct 31, 2023
FullteaR pushed a commit to FullteaR/cpython that referenced this pull request Nov 3, 2023
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants